-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Add Unittests #54
Conversation
__tests__/unittest/defaults.test.js
Outdated
const test$schemaDefaultValue = 'https://sap.github.io/open-resource-discovery/spec-v1/interfaces/Document.schema.json'; | ||
it('should return default value', () => { | ||
expect(defaults.$schema).toEqual(test$schemaDefaultValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's have a meeting on this how we generally want to write unit-tests.
I think a test like this is not giving us more value than a snapshot test (that we already have), but is more cumbersome to maintain. For the "isEqual" tests, I think we can just rely on the Snapshot test to ensure that we don't accidentally change something without first reviewing and accepting the change.
For the real unit-tests, I would prefer to have them test more logical things, like that there's no "undefined" string somewhere in a generated ORD document (we had such issues creep in before), or that the ORD ID of the package assignment can be resolved within the ORD document.
We could test equals assumptions where it's really critical or has special meaning, like that the default policy level of default is "sap:base:v1".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Fannon, thank you for valuable comments. We need a meeting discussing about unittest and code style with team agreement. From my understanding and experiences, uniitest should be simple, quick, fragile, readable and understandable. If the new codes break the unittest, we have a chance to think twice, either the issue from codes or tests. This is the front safeguard for us. And maintaining unittest should be as same important as production code, we should not worry about maintaining test.
The "snapshot test" you mentioned is more like integration test for me. The purpose of integration test is to watch how it looks like in general at the end with different environment settings or complex scenarios, which unittest can not accomplish.
Comparing with snapshot test, I feel the test you mentioned has more values in different scenarios:
- The fine-grained unittest is easier and quicker to locate the issue. And during development, we can also run the fine-grained test independently, I think it's better to debug as well.
- Later, I want to clean and refactor the code for fixing naming issue, reducing function complexity and duplicable. The fine-grained unittest can support this process.
- I have same feeling with you. This PR is more like uniitest proposal and skeleton. We need to improve it with more test cases and more logics. Later, I'm pretty sure, we can reuse snapshot to reduce the line of codes. Back to this test, the target function is simply enough, I assume more flexible and testcases will be enough.
btw:
I found the bug undefined during writing unittest:
ord/__tests__/unittest/templates.test.js
Line 20 in b914cfa
// TODO: fix undefined. Root cause: get the value from ${entity['@ODM.entityName']} without setting default value |
No description provided.